-
Notifications
You must be signed in to change notification settings - Fork 14.1k
rustdoc: Add unstable --merge-doctests=yes/no/auto flag
#149565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
r? @notriddle rustbot has assigned @notriddle. Use |
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
--merge-doctests=yes/no flag--merge-doctests=yes/no/auto flag
dde2ec7 to
0ff6d88
Compare
| doctest_bundle_2018.rs:$LINE:$COL: error: `unmangled_name` redeclared with a different signature: this signature doesn't match the previous declaration | ||
| error: aborting due to 1 previous error | ||
| error: failed to merge doctests | ||
| | | ||
| = note: requested explicitly on the command line with `--merge-doctests=yes` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this error kinda sucks, but this is an unstable feature anyway, and with -C save-temps people will actually be able to inspect the merged code, which should make this easier to debug.
oh, maybe this should recommend -C save-temps as a note?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
hm this broke the reference’s doctest somehow |
|
i'm completely unable to reproduce this :/ i see ci has this output which i don't see locally: |
|
i ran |
b1e2ca9 to
e2aa3fc
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
src/librustdoc/doctest/make.rs
Outdated
| && !(has_macro_def && everything_else.contains("$crate")); | ||
| can_merge | ||
| } else { | ||
| can_merge_doctests != MergeDoctests::Never |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I reading this correctly that this overrides standalone_crate?
From the PR description (emphasis mine)
This is useful for changing the default for whether doctests are merged or not.
For me, default implies that this is what we do in the absence of other information. standalone_crate is an obvious one for being respected. compile_fail is another. test_harness I'm unsure about.
Now, for any of the detection based on the code (global allocator, crate attrs, macros, $crate, etc, I see that being part of the autodetection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case it is relevant, https://github.com/epage/rust/commits/merged_crate is where I left off on my work on the attribute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent catch, omg. fixed this and added a test.
This comment has been minimized.
This comment has been minimized.
|
something really weird is going on with compiletest; locally it applies the for now I'm just going to change the normalization to match compiletest's normalization. |
|
oh, i wonder if this is about absolute vs relative paths :/ |
|
i think the proper fix here is that rustdoc should use |
This is useful for changing the *default* for whether doctests are merged or not. Currently, that default is solely controlled by `edition = 2024`, which adds a high switching cost to get doctest merging. This flag allows opt-ing in even on earlier additions. Unlike the `edition = 2024` default, `--merge-doctests=yes` gives a hard error if merging fails instead of falling back to running standalone tests. The user has explicitly said they want merging, so we shouldn't silently do something else. `--merge-doctests=auto` is equivalent to the current 2024 edition behavior, but available on earlier editions.
This allows viewing failed merged doctests.
4a30a64 to
646667a
Compare
646667a to
415953a
Compare
| && crate_attrs.is_empty() | ||
| // We try to look at the contents of the test to detect whether it should be merged. | ||
| // This is not a complete list of possible failures, but it catches many cases. | ||
| let will_probably_fail = has_global_allocator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know why I'm getting a chuckle at that variable name :)
This is useful for changing the default for whether doctests are merged or not. Currently, that default is solely controlled by
edition = 2024, which adds a high switching cost to get doctest merging. This flag allows opting in even on earlier editions.Unlike the
edition = 2024default,--merge-doctests=yesgives a hard error if merging fails instead of falling back to running standalone tests. The user has explicitly said they want merging, so we shouldn't silently do something else.--merge-doctests=autois equivalent to the current 2024 edition behavior, but available on earlier editions.Helps with #141240. @epage said in that issue he would like a per-doctest opt-in, and that seems useful to me in addition to this flag, but I think it's a separate use case from changing the default.